Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(fabric.Path): setting path during runtime #7141

Merged
merged 4 commits into from
Oct 30, 2021
Merged

fix(fabric.Path): setting path during runtime #7141

merged 4 commits into from
Oct 30, 2021

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Jun 21, 2021 •
edited
Loading

This PR with #7140 finalizes the replacement of #7082

Apparently calling setCoords was redundant.
Everything looks good

@ShaMan123 ShaMan123 changed the title feat: Update Path#path during runtime fix(fabric.Path): setting path during runtime Jun 21, 2021
ShaMan123
ShaMan123 commented Jun 21, 2021
View reviewed changes
src/shapes/path.class.js
@@ -66,7 +73,19 @@
if (!this.path) {
return;
}
fabric.Polyline.prototype._setPositionDimensions.call(this, options);
fabric.Polyline.prototype._setPositionDimensions.call(this, options || {});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to extract _setPositionDimensions from Polyline?

@ShaMan123 ShaMan123 mentioned this pull request Jun 26, 2021
Closed
@stale
Copy link

stale bot commented Jul 5, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue marked as stale by the stale bot label Jul 5, 2021
@ShaMan123
Copy link
Contributor Author

@asturur merge?

@stale stale bot removed the stale Issue marked as stale by the stale bot label Jul 6, 2021
@stale
Copy link

stale bot commented Jul 20, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue marked as stale by the stale bot label Jul 20, 2021
@ShaMan123
Copy link
Contributor Author

@asturur

@stale stale bot removed the stale Issue marked as stale by the stale bot label Jul 23, 2021
@ShaMan123
Copy link
Contributor Author

stale bot 😠

@ShaMan123 ShaMan123 mentioned this pull request Aug 14, 2021
Closed
@stale
Copy link

stale bot commented Aug 15, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue marked as stale by the stale bot label Aug 15, 2021
@stale stale bot closed this Aug 22, 2021
@ShaMan123
Copy link
Contributor Author

@asturur ?

@asturur asturur reopened this Sep 9, 2021
@stale stale bot removed the stale Issue marked as stale by the stale bot label Sep 9, 2021
@asturur
Copy link
Member

asturur commented Sep 19, 2021

Ok i finally found the time to come back here.
I had many thoughts on this change and there are some thing we shouldn't really do.

A fabric.Path is defined by its own path data, and updating it is not so much common.
We shouldn't execute that logic with the set command.
I would prefer we get an imperative this.updatePath or this.changePath method.

If you want we can merge this PR removing the changes to _set/set, and is good to go.
Or we can work on adding changePath in this PR too ( honestly i would merge and reiterate ).
updatePath, changePath, should take of course path as an argument, and another argument to specify a point of the path that won't change position.

Changing a path data with something entirely different does not make sense and we can't really fix that.
But changing a path with another where only an handful of points have been moved is a more common use case, and the hard part is always having the 2 render in sequence look good

@stale
Copy link

stale bot commented Oct 4, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue marked as stale by the stale bot label Oct 4, 2021
@stale stale bot removed the stale Issue marked as stale by the stale bot label Oct 5, 2021
@ShaMan123
Copy link
Contributor Author

Done

@asturur
Copy link
Member

asturur commented Oct 5, 2021

i will add the changePath method with the optional anchor point

@stale
Copy link

stale bot commented Oct 20, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue marked as stale by the stale bot label Oct 20, 2021
@ShaMan123
Copy link
Contributor Author

not stale

@stale stale bot removed the stale Issue marked as stale by the stale bot label Oct 25, 2021
@asturur
Copy link
Member

asturur commented Oct 30, 2021

i forgot to merge this?

@asturur
Copy link
Member

asturur commented Oct 30, 2021

now i have as a task to write the function that call _setPath but also keep the position steady.

@asturur asturur merged commit ab168bb into fabricjs:master Oct 30, 2021
@asturur asturur mentioned this pull request Oct 30, 2021
Closed
@ShaMan123 ShaMan123 deleted the update-path-next branch October 31, 2021 04:55
This was referenced Nov 15, 2021
Closed
Closed
rockerBOO pushed a commit to rockerBOO/fabric.js that referenced this pull request Nov 27, 2021
@ShaMan123 @rockerBOO
fix(fabric.Path): setting path during runtime (fabricjs#7141)
b0f635f
@asturur asturur mentioned this pull request Jan 26, 2022
Merged
rockerBOO pushed a commit to rockerBOO/fabric.js that referenced this pull request May 2, 2022
@ShaMan123 @rockerBOO
fix(fabric.Path): setting path during runtime (fabricjs#7141)
769a37a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ShaMan123 @asturur